Conversation
…pdate test cases, UI, and dependencies
…sage receiving and file sending
…ssage` and corresponding unit tests
…d Ntfy connectivity adjustments
…for default and custom topics
WalkthroughAdds an ntfy-based chat feature: new connection interface and HTTP implementation, message DTO, model refactor for sending/receiving messages and files, updated controller and FXML for a chat UI with attachments, Maven/module updates (dotenv, Jackson, and preview compiler), plus unit tests and a test spy. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant HC as HelloController
participant HM as HelloModel
participant NC as NtfyConnectionImpl
participant Server as Ntfy Server
rect rgb(200,220,255)
note right of User: Send text message
User->>HC: click Send
HC->>HM: sendMessage()
HM->>HM: create local NtfyMessageDto (isLocal=true)
HM->>HC: update messages (runOnFx)
HM->>NC: send(message, topic)
NC->>Server: POST /{topic}
Server-->>NC: 2xx
NC-->>HM: success
HM->>HC: clear input
end
rect rgb(220,240,220)
note right of Server: Incoming event stream
Server->>NC: stream JSON lines (event="message")
NC->>NC: parse -> NtfyMessageDto
NC->>HM: messageHandler.accept(dto)
HM->>HM: add to messages (runOnFx)
HM->>HC: UI updated (ListView)
end
rect rgb(255,240,200)
note right of User: Send file flow
User->>HC: attachFile()
HC->>HM: setFileToSend(file)
User->>HC: click Send
HC->>HM: sendFile()
HM->>HM: create local file NtfyMessageDto (isLocal=true)
HM->>NC: sendFile(file, topic)
NC->>Server: POST /{topic} (file + headers)
Server-->>NC: 2xx
NC-->>HM: success
HM->>HC: clear file attachment
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (9)
src/main/java/com/example/HelloFX.java (1)
14-16: Remove unnecessary blank lines.Multiple consecutive blank lines reduce readability.
@Override public void start(Stage stage) throws Exception { - - - FXMLLoader fxmlLoader = new FXMLLoader(HelloFX.class.getResource("hello-view.fxml"));src/main/resources/com/example/hello-view.fxml (1)
14-20: Consider English comments for broader accessibility.The comments are in Swedish (e.g., "VBox för att hålla BIFOGAD FIL-STATUS"). For better code maintainability in international teams, consider using English comments.
src/test/java/com/example/HelloModelTest.java (3)
38-46: Consider English comments for broader accessibility.The comments contain Swedish text (e.g., "Verifiera att anslutningen anropades med rätt meddelande"). For better maintainability, consider using English.
86-87: Consider English comments.Comment "KORRIGERING: Använder NtfyMessageDto.message()" is in Swedish.
9-13: Remove unused I/O imports.All I/O-related imports are unused in this test:
File,FileNotFoundException,IOException,Files, andPath.-import java.io.File; -import java.io.FileNotFoundException; -import java.io.IOException; -import java.nio.file.Files; -import java.nio.file.Path;src/main/java/com/example/NtfyMessageDto.java (1)
3-5: Remove unnecessary blank lines.Multiple consecutive blank lines before the class declaration are unnecessary.
package com.example; - - - public record NtfyMessageDto(String id, long time, String event, String topic, String message, boolean isLocal) {src/test/java/com/example/NtfyConnectionSpy.java (2)
12-12: Consider making messageHandler private.The
messageHandlerfield has package-private access, which is accessed directly inHelloModelTest(line 83). This breaks encapsulation and makes the test brittle.Use the existing
simulateMessageReceived()method instead of direct field access. Update the test:In
HelloModelTest.java:- spy.messageHandler.accept(testDto); + spy.simulateMessageReceived(testDto);Then make the field private in this class:
- Consumer<NtfyMessageDto> messageHandler = null; + private Consumer<NtfyMessageDto> messageHandler = null;
15-25: Consider English comments for broader accessibility.The comments are in Swedish. For better maintainability in international teams, consider using English throughout the codebase.
Also applies to: 66-69
pom.xml (1)
44-48: Use the latest stable WireMock version.The project depends on a beta version (
4.0.0-beta.15). The latest stable WireMock release is 3.13.1. Consider updating to this stable version for production reliability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
.gitignore(1 hunks)pom.xml(2 hunks)src/main/java/com/example/HelloController.java(1 hunks)src/main/java/com/example/HelloFX.java(2 hunks)src/main/java/com/example/HelloModel.java(1 hunks)src/main/java/com/example/NtfyConnection.java(1 hunks)src/main/java/com/example/NtfyConnectionImpl.java(1 hunks)src/main/java/com/example/NtfyMessageDto.java(1 hunks)src/main/java/module-info.java(1 hunks)src/main/resources/com/example/hello-view.fxml(1 hunks)src/test/java/com/example/HelloModelTest.java(1 hunks)src/test/java/com/example/NtfyConnectionSpy.java(1 hunks)
🔇 Additional comments (10)
.gitignore (1)
3-3: ✓ Appropriate environment configuration security practice.Adding
.envto.gitignorealigns well with the introduction of environment-based configuration in this PR. This prevents accidentally committing sensitive configuration (hostnames, API keys, etc.) to version control.src/main/java/module-info.java (1)
5-7: LGTM!The new module dependencies appropriately support the Ntfy integration functionality (HTTP client, JSON parsing, and environment configuration).
src/main/resources/com/example/hello-view.fxml (1)
1-67: LGTM!The UI redesign is well-structured with proper BorderPane layout and JavaFX bindings. The fx:id references and event handlers align with the controller implementation.
src/test/java/com/example/HelloModelTest.java (3)
25-46: LGTM!The test correctly verifies that calling
sendMessage()triggers the connection with the expected message, updates the model's observable list, and marks the message as locally sent.
48-61: Test validates HTTP interaction correctly.The test properly uses WireMock to stub the endpoint and verify the POST request was made with the correct body.
66-88: Test correctly validates message reception flow.The test properly verifies that received messages are added to the observable list via the spy's message handler.
src/main/java/com/example/NtfyConnection.java (1)
7-19: Interface contract is well-defined.The interface provides a clear contract for Ntfy messaging operations including connection, sending messages/files, and receiving messages.
src/main/java/com/example/NtfyMessageDto.java (1)
6-20: LGTM!The record structure is well-designed for a DTO with appropriate constructors for different use cases and a sensible
toString()implementation.src/test/java/com/example/NtfyConnectionSpy.java (2)
7-105: Test spy implementation is functional.The spy correctly tracks method calls and simulates message reception for testing purposes.
43-45: Current implementation is appropriate; method is correctly documented as unused but required by interface contract.Verification confirms the
receive()method is indeed unused throughout the codebase—no callers were found. However, since it's an@Overrideof theNtfyConnectioninterface method, it must remain to satisfy the interface contract. The existing comment correctly explains this necessity. No action required.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/test/java/com/example/HelloModelTest.java (4)
7-7: Remove unused import.The
@ExtendWithimport is not used in this test class. The@WireMockTestannotation already provides the necessary JUnit extension.Apply this diff:
-import org.junit.jupiter.api.extension.ExtendWith;
18-40: Maintain consistent language in comments.The test includes Swedish comments (lines 32, 35, 38) mixed with English code and annotations. Additionally, line 18 contains an unnecessary comment that adds no value.
Apply this diff to use English consistently and remove the unnecessary comment:
- // I HelloModelTest.java @Test @DisplayName("Given a model with messageToSend when calling sendMessage then send method on connection is called") void sendMessageCallsConnectionWithMsgToSend() { // Arrange - Given var spy = new NtfyConnectionSpy(); var model = new HelloModel(spy); String expectedMessage = "Hello World"; model.setMessageToSend(expectedMessage); // Act - When model.sendMessage(); // Assert - Then - // 1. Verifiera att anslutningen anropades med rätt meddelande: + // 1. Verify that the connection was called with the correct message: assertThat(spy.getLastSentMessage()).isEqualTo(expectedMessage); - // 2. Verifiera att det skickade meddelandet lades till i modellens lista (lokal uppdatering): + // 2. Verify that the sent message was added to the model's list (local update): assertThat(model.getMessages()).hasSize(1); assertThat(model.getMessages().get(0).message()).isEqualTo(expectedMessage); - // Verifiera även att det markerades som lokalt skickat + // Verify that it was marked as locally sent assertThat(model.getMessages().get(0).isLocal()).isTrue(); }
42-55: Add@DisplayNameannotation for consistency and improve formatting.This test is missing the
@DisplayNameannotation that the other tests use, and contains minor formatting inconsistencies.Apply this diff:
@Test + @DisplayName("Given a model with connection to fake server when sending message then POST request is made to server") void sendMessageToFakeServer(WireMockRuntimeInfo wmRuntimeInfo) { var con = new NtfyConnectionImpl("http://localhost:" + wmRuntimeInfo.getHttpPort()); stubFor(post("/" + DEFAULT_TOPIC).willReturn(ok())); var model = new HelloModel(con); model.setMessageToSend("Hello World"); - model.sendMessage(); - //Verify call made to server + // Verify call made to server verify(postRequestedFor(urlEqualTo("/" + DEFAULT_TOPIC)) .withRequestBody(matching("Hello World"))); }
60-82: Add@DisplayName, improve encapsulation, and maintain consistent language.This test has several issues:
- Missing
@DisplayNameannotation (inconsistent with test structure)- Direct access to
spy.messageHandlerbreaks encapsulation and tightly couples the test to the spy's internal implementation- Swedish comment on line 80
- Minor formatting issue on line 67 (missing space before
=)Consider adding a method to
NtfyConnectionSpylikesimulateReceivedMessage(NtfyMessageDto dto)to encapsulate the handler invocation. For now, apply this diff:@Test + @DisplayName("Given a model with spy connection when message is received then message is added to observable list") void receiveMessageIsAddedToObservableList() { - //Arrange Given + // Arrange - Given var spy = new NtfyConnectionSpy(); var model = new HelloModel(spy); var testMessage = "This is a test message"; - var testDto=new NtfyMessageDto( + var testDto = new NtfyMessageDto( "id-123", System.currentTimeMillis(), "message", DEFAULT_TOPIC, testMessage); assertThat(model.getMessages()).isEmpty(); - //Act When + // Act - When spy.messageHandler.accept(testDto); - //Assert Then + + // Assert - Then assertThat(model.getMessages()).hasSize(1); - // KORRIGERING: Använder NtfyMessageDto.message() assertThat(model.getMessages().get(0).message()).isEqualTo(testMessage); } - - -Note: Consider refactoring
NtfyConnectionSpyto provide a method likesimulateReceivedMessage(NtfyMessageDto)instead of exposing themessageHandlerfield directly.
annikaholmqvist94
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/main/java/com/example/HelloModel.java (2)
60-83: Moveconnection.send(...)off the JavaFX Application Thread.
sendMessage()currently callsconnection.send(message, currentTopic.get())directly after updating the UI viarunOnFx(...). With the defaultNtfyConnectionImpl, this ends up in a blockingHttpClient.send(...)call. When invoked from the controller’s@FXMLhandler, that means the FX Application Thread will block until the HTTP request completes, freezing the UI on slow or flaky connections.Push the network call onto a background executor and keep only the UI list-update and input clearing on the FX thread.
- // 3. Skicka meddelandet via anslutningen - connection.send(message, currentTopic.get()); - - // 4. Rensa meddelandefältet efter skickning - messageToSend.set(""); + // 3. Skicka meddelandet via anslutningen i bakgrunden + CompletableFuture.runAsync(() -> connection.send(message, currentTopic.get())); + + // 4. Rensa meddelandefältet efter skickning (på FX‑tråden) + messageToSend.set("");Confirm that `HttpClient.send(...)` is a blocking call in the standard Java HTTP client and that using `CompletableFuture.runAsync(...)` is appropriate to avoid blocking the JavaFX Application Thread.
90-113: Apply the same async pattern tosendFile()to avoid UI freezes during uploads.
sendFile()mirrorssendMessage()and callsconnection.sendFile(file, currentTopic.get())synchronously. File uploads can be significantly slower than text messages, so this is even more likely to freeze the UI if called from the FX thread.Use the same background execution pattern here.
- // 3. Skicka filen - connection.sendFile(file, currentTopic.get()); - - // 4. Rensa filbilagan efter skickning - fileToSend.set(null); + // 3. Skicka filen i bakgrunden + CompletableFuture.runAsync(() -> connection.sendFile(file, currentTopic.get())); + + // 4. Rensa filbilagan efter skickning (på FX‑tråden) + fileToSend.set(null);Check best practices for performing blocking I/O (like `HttpClient.send` and file uploads) from a JavaFX application, specifically around keeping the FX Application Thread responsive.
🧹 Nitpick comments (4)
src/main/java/com/example/HelloController.java (2)
131-155: Avoid potentialNullPointerExceptionwhen checking event type inSimpleMessageCell.
item.event().equals("file")will NPE ifeventever comes back null from the server or from a test DTO. Defensive coding here is cheap and keeps the cell renderer resilient.- String displayMessage = item.message() != null && !item.message().trim().isEmpty() - ? item.message() - : (item.event().equals("file") ? "[File Uploaded]" : ""); + String displayMessage = item.message() != null && !item.message().trim().isEmpty() + ? item.message() + : ("file".equals(item.event()) ? "[File Uploaded]" : "");
183-196: Use a control that is guaranteed to exist for resolving theStageinattachFile().
Stage stage = (Stage) (chatListView.getScene().getWindow());couples this handler tochatListViewalways being present and attached to a Scene. If the layout changes, this can throw aNullPointerException.Using the button that triggers the action is usually safer:
- Stage stage = (Stage) (chatListView.getScene().getWindow()); + Stage stage = (Stage) (attachFile.getScene().getWindow());src/main/java/com/example/NtfyConnectionImpl.java (2)
138-174:sendFile(...)implementation is straightforward; consider aligning error messaging withsend(...).The file upload method validates the file, sets a reasonable fallback content type, sanitizes the filename header, and returns a boolean based on HTTP status. This is a good minimal implementation.
To keep behavior consistent with
send(...), you might eventually want to standardize logging (e.g., include topic and status code on failures), but that’s optional for this lab.
61-94: Improve robustness and observability inconnect(...)when parsing the stream.The streaming subscription logic is correct for the Ntfy API, but two improvements would make it more diagnosable and defensive:
- JSON parse failures are currently silently dropped (returning null); logging the offending line would help debugging downstream issues.
- The filter
message -> message.event().equals("message")could safely use defensive null-check style,"message".equals(message.event()), to prevent NPE if malformed data reaches this point.- .map(s -> { - try { - return mapper.readValue(s, NtfyMessageDto.class); - } catch (JsonProcessingException e) { - return null; - } - }) + .map(s -> { + try { + return mapper.readValue(s, NtfyMessageDto.class); + } catch (JsonProcessingException e) { + System.out.println("Failed to parse message JSON: " + e.getMessage()); + return null; + } + }) @@ - .filter(Objects::nonNull) - .filter(message -> message.event().equals("message")) + .filter(Objects::nonNull) + .filter(message -> "message".equals(message.event()))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/main/java/com/example/HelloController.java(1 hunks)src/main/java/com/example/HelloModel.java(1 hunks)src/main/java/com/example/NtfyConnectionImpl.java(1 hunks)src/test/java/com/example/NtfyConnectionSpy.java(1 hunks)
🔇 Additional comments (3)
src/main/java/com/example/HelloController.java (1)
74-88: Send button enable/disable binding is well thought out.The binding that only disables the send button when both the text is empty and no file is attached matches the intended UX and is implemented cleanly with
Bindings.createBooleanBinding.src/main/java/com/example/HelloModel.java (1)
47-54:runOnFxhelper is a solid approach for UI-safe updates and testability.The
runOnFxutility that falls back to a directtask.run()when the JavaFX platform isn’t initialized makes the model usable in tests without spinning up JavaFX, while still keeping real UI updates on the FX thread.src/test/java/com/example/NtfyConnectionSpy.java (1)
7-105: Test spy design looks appropriate for verifying model–connection interactions.The spy tracks connect calls, last sent message/topic/file, and lets tests inject received messages via
simulateMessageReceived(...). This covers the behaviors HelloModel relies on without involving a real server.
… improve `NtfyMessageDto` constructors for clarity and consistency. Clean up unused dependencies and imports.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
src/main/java/com/example/NtfyConnection.java (1)
7-22: Minor: Clean up formatting and redundant modifiers.The interface has some minor style issues:
- Line 19: The
publicmodifier is redundant (interface methods are implicitly public)- Multiple unnecessary blank lines (17-18, 21)
Apply this diff:
void connect(String topic, Consumer<NtfyMessageDto> messageHandler); - - - public void receive(Consumer<NtfyMessageDto> messageHandler); - - + void receive(Consumer<NtfyMessageDto> messageHandler); }src/main/java/com/example/NtfyMessageDto.java (3)
1-5: Remove excessive blank lines.Lines 3-5 contain unnecessary blank lines at the top of the file.
Apply this diff:
package com.example; - - - public record NtfyMessageDto(String id, long time, String event, String topic, String type, String message, boolean isLocal) {
8-28: Consider using English for comments and documentation.The codebase uses English for all identifiers (class names, method names, variables), but Swedish is used for comments and Javadoc (lines 8, 13-19, 21-22, 25). This creates an inconsistency that can make the code harder to maintain for non-Swedish speakers.
20-23: Remove misleading comment about casting.Line 21-22 states "Vi använder 0L för att explicit kasta 0 som en long" (We use 0L to explicitly cast 0 as a long). This is misleading—
0Lis a long literal, not a cast. No casting occurs here.Apply this diff:
public NtfyMessageDto(String message, String topic, String type, boolean isLocal) { - // Fix: Vi använder 0L för att explicit kasta 0 som en long this(null, 0L, "message", topic, type, message, isLocal); }src/main/java/com/example/HelloModel.java (1)
98-99: Remove duplicate comment.Lines 98-99 contain an identical comment that appears twice in a row.
Apply this diff:
}); - // 2. Utför nätverksanropet på en bakgrundstråd - // 2. Utför nätverksanropet på en bakgrundstråd CompletableFuture.runAsync(() -> {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
pom.xml(2 hunks)src/main/java/com/example/HelloFX.java(2 hunks)src/main/java/com/example/HelloModel.java(1 hunks)src/main/java/com/example/NtfyConnection.java(1 hunks)src/main/java/com/example/NtfyMessageDto.java(1 hunks)src/test/java/com/example/NtfyConnectionSpy.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main/java/com/example/HelloFX.java
- src/test/java/com/example/NtfyConnectionSpy.java
🔇 Additional comments (1)
pom.xml (1)
43-52: Both dependency versions are valid and free from known vulnerabilities.
- jackson-databind 2.19.0: Version exists and is outside all known vulnerable ranges (which target versions 2.0–2.13.x). No security issues.
- dotenv-java 3.2.0: Version exists with no known security vulnerabilities.
No action required.
…dling for message/file sending in `HelloModel`.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
src/main/java/com/example/HelloController.java (1)
47-74: Prefer private FXML fields for consistency and encapsulation.
chatListView,topicLabel, andattachedFileLabelarepublicwhile the other injected fields (sendButton,messageInput,attachFile) areprivate. JavaFX injection does not require these fields to be public; making themprivatekeeps the controller’s surface smaller and more consistent.This is not blocking, but worth aligning for readability and encapsulation.
src/main/java/com/example/HelloModel.java (1)
42-54:getGreeting()andrunOnFx()helper are reasonable.
getGreeting()returning a constant is fine for now and keeps the controller simple.runOnFx()correctly runs tasks on the FX thread when available and falls back to direct execution when the platform is not initialized (useful for tests).Be aware that the
catch (IllegalStateException)will also catch any otherIllegalStateExceptionfrom insidetask.run(), but in this context that’s an acceptable trade-off.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/main/java/com/example/HelloController.java(1 hunks)src/main/java/com/example/HelloModel.java(1 hunks)
🔇 Additional comments (7)
src/main/java/com/example/HelloController.java (3)
23-41: HOST_NAME validation and model wiring look correct.Reading
HOST_NAMEonce and failing fast in the static block before constructingNtfyConnectionImpl(HOST_NAME)is a solid improvement over the previous unsafe usage; it will surface misconfiguration early and clearly.
81-133: UI bindings ininitialize()are coherent and defensive.The binding logic for
sendButton,topicLabel,attachedFileLabel,messageInput, andchatListViewis well structured:
sendButton.disableProperty()is only true when both the message is empty and no file is attached, which matches the intended UX.messageInputis correctly bound bidirectionally tomodel.messageToSendProperty().attachedFileLabellistens tofileToSendProperty()and updates both text and style.chatListViewis wired tomodel.getMessages()and uses the custom cell.The extra null checks around controls make the controller more robust when reused with different FXMLs.
172-185:sendMessage()delegation and focus handling are fine.Routing to
model.sendFile()when a file is attached and tomodel.sendMessage()otherwise is clear, and restoring focus tomessageInput(when present) is a nice usability touch. No issues here.src/main/java/com/example/HelloModel.java (4)
24-37: Model initialization and connection wiring look good.Storing the
NtfyConnection, seedingcurrentTopicwith"mytopic", and immediately callingconnection.connect(currentTopic.get(), this::receiveMessage)gives a clear lifecycle: the model owns the connection and pushes incoming messages into its observable list. This fits well with the controller bindings.
121-131: File attachment property API is clean and matches controller usage.
fileToSendProperty()andsetFileToSend(File)give a straightforward API for the controller to manage attachments, and the internal usage insendFile()plus the controller’s label listener are consistent.
138-149: Incoming message handling viareceiveMessageis appropriate.Using
receiveMessage(NtfyMessageDto message)to add messages to the observable list viarunOnFxkeeps all list mutations on the FX thread (or inline for tests). This matches how the controller bindschatListViewand avoids typical JavaFX threading issues.
147-174: Property accessors for messages, text, and topic are straightforward.
getMessages(),messageToSendProperty(),currentTopicProperty(), andsetMessageToSend(String)are simple and correctly expose the underlying properties for binding in the controller. No issues here.
…handling for UI readiness check, and refine async logic in `HelloModel`.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/main/java/com/example/HelloController.java (2)
107-122: Consider extracting inline styles to CSS classes.The inline styles for
attachedFileLabelwork correctly but could be moved to an external stylesheet for better maintainability and reusability.For example, in your CSS file:
.file-attached { -fx-font-style: italic; -fx-font-size: 12px; -fx-text-fill: #008000; } .file-not-attached { -fx-font-style: italic; -fx-font-size: 12px; -fx-text-fill: #333; }Then update the listener:
if (newFile != null) { attachedFileLabel.setText("Attached file: " + newFile.getName()); - attachedFileLabel.setStyle("-fx-font-style: italic;" + - " -fx-font-size: 12px;" + - " -fx-text-fill: #008000;"); + attachedFileLabel.getStyleClass().setAll("file-attached"); } else { attachedFileLabel.setText("No file attached"); - attachedFileLabel.setStyle("-fx-font-style: italic; " + - "-fx-font-size: 12px; " + - "-fx-text-fill: #333;"); + attachedFileLabel.getStyleClass().setAll("file-not-attached"); }
192-212: LGTM! Null safety checks properly implemented.The defensive checks at Lines 195-198 prevent NPE when the UI isn't ready, and using
attachFile.getScene().getWindow()is more reliable than usingchatListView. This addresses the previous concern.Consider adding file size validation to prevent users from selecting excessively large files:
File selectedFile = fileChooser.showOpenDialog(stage); if (selectedFile != null) { long maxSize = 50 * 1024 * 1024; // 50 MB if (selectedFile.length() > maxSize) { System.err.println("File too large: " + selectedFile.length() + " bytes (max: " + maxSize + " bytes)"); // Optionally show an Alert dialog to the user return; } model.setFileToSend(selectedFile); }src/main/java/com/example/HelloModel.java (2)
60-84: Async network call properly implemented.The
CompletableFuture.runAsyncpattern correctly moves the blocking HTTP request off the JavaFX Application Thread, preventing UI freezes. The duplicate send bug from previous reviews has been fixed.The error handling could be enhanced to revert the optimistic UI update when the send fails:
CompletableFuture.runAsync(() -> { try { connection.send(message, currentTopic.get()); } catch (Exception e) { System.err.println("Failed to send message: " + e.getMessage()); + // Revert optimistic update + runOnFx(() -> messages.remove(localSentDto)); } });This ensures the UI accurately reflects the actual send status rather than showing "sent" for failed messages.
91-113: Async file upload properly implemented.The pattern correctly mirrors
sendMessage(), with network I/O on a background thread and UI updates on the FX thread. The duplicate send bug has been fixed.Similar to
sendMessage(), consider reverting the optimistic update on failure:CompletableFuture.runAsync(() -> { try { connection.sendFile(file, currentTopic.get()); } catch (Exception e) { System.err.println("Failed to send file: " + e.getMessage()); + // Revert optimistic update and restore attachment + runOnFx(() -> { + messages.remove(localSentDto); + fileToSend.set(file); + }); } });This provides better user feedback when file uploads fail.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/main/java/com/example/HelloController.java(1 hunks)src/main/java/com/example/HelloModel.java(1 hunks)
🔇 Additional comments (3)
src/main/java/com/example/HelloController.java (2)
23-31: LGTM! Environment variable validation properly implemented.The static validation block ensures
HOST_NAMEis set before the class is used, with a clear error message. This addresses the previous concern about fail-fast configuration.
139-165: LGTM! Null-safe event comparison implemented.The constant-first comparison
"file".equals(item.event())at Line 153 prevents potential NPE ifevent()returns null. Good defensive coding.src/main/java/com/example/HelloModel.java (1)
47-54: LGTM! Test-friendly FX thread helper.The
runOnFxmethod handles both production (JavaFX) and test environments gracefully by catchingIllegalStateExceptionwhen the FX toolkit isn't initialized.
annikaholmqvist94
changed the title
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores